I not sure what people would set AHRS_WIND_MAX as, but I guess to be most effective it should be fairly close to maximum expected wind speeds. Maybe this should to re-enable the airspeed sensor if the wind-speed reading drops bellow AHRS_WIND_MAX again? So as not to be permanently disabled in a gust, but then again that could cause some funny behaviour if the sensor has genuinely failed and it just outputting nonsense.

edit: maybe we should flash a warning, 'Airspeed Disabled, max wind' or something

@IamPete1 It puts out a gcs warning message: "Airspeed sensor - possibly faulty - ARSPED_USE: 0"

ah, i see you mention your going to add the re - enabling feature, and you already have the warnings in the code. Looks like you have it under control, I should have read it properly.

Logic looks fine, the only possibly issue is AP::gps().ground_speed(), can this be negative? It may be positive only so it would mess up your calc if you were going backwards in headwind. I think you only want the forwards component of your ground speed, although that sort of depends how directional the sensors are.

@IamPete1 From memory its positive and the ground course would be reversed. I'll need to check that though. Good pickup.

How does this interact with airspeed blending and dual airspeed sensors? If possible, it'd be useful to have it monitor each sensor independently, so if one fails and outputs an unrealistic value, it can default to the other sensor instead of disabling both.

Specifically, @lowlyelevated is referring to this merged pull request, which uses healthy/ not healthy flags for multiple airspeed sensors to switch between them. What's unclear to me is whether it will switch to synthetic airspeed if both are marked unhealthy . . .

Ground speed and wind speed are vectors. I doubt that simple addition can be used with respect to airspeed comparison.

IMO the entire idea of AHRS_WIND_MAX only works if you set a very large floor value. I've seen people be wrong at what the winds are aloft far more often, in which case you would want to set a very conservative estimate (IE something that is very far outside of plausible, so if you would be happy in flying upto 13 m/s wind, then the limit should be set up at 20 m/s or more). This only becomes useful once it's extremely far outside the tuned envelope. In my opinion we should instead be assessing if the wind estimation we have is physically consistent with what the aircraft is observing from the sensors and looking for a divergence.

@ChrisBird as a general note using rand() for wind simulation via the airspeed reading is only a test of our ability to reject pseduorandom noise through the airspeed sensor. It doesn't allow the vehicle to get a decent wind simulation or estimation as it is not physically accurate. (That said a small low level of noise is realistic). As far as testing in SITL you should have been able to test this by simply increasing or decreasing the wind in the simulation. If you want to introduce error into the sensor in flight the best way is to simply modulate the ARSPD_OFFSET parameter as we simulate an analog sensor in SITL.

Specifically, @lowlyelevated is referring to this merged pull request, which uses healthy/ not healthy flags for multiple airspeed sensors to switch between them. What's unclear to me is whether it will switch to synthetic airspeed if both are marked unhealthy . . .

To the best of my knowledge that was reverted. Or if it wasn't it has under seen really heavy refactoring since then and I can no longer find it in the code.

@EShamaev gps ground speed is a float, there is also ground course which gives it a direction.

@WickedShell i agree on the setting of wind_max. In the case of one log i reviewed it reported values between 16 and 19m/s while having a ground speed of over 40m/s, throttle maxed and it was evident from the pattern it was flying that the wind was at most a few m/s. This needs to work and not trip failure when the sensor is behaving. I'd love to review more logs with this issue. Agreed on the better solution, thats why ive said this is not the end solution. Will have a play with the sitl suggestion, can be auto tested then.

I still feel that using only values (float) for ground speed and wind speed is not correct if you compare it to airspeed and also make decision to mark sensor failure. They are vectors and almost never these three are collinear allowing to use only values.

#7738 was reverted,
there is #7891 which is WIP and more check were added.

I think we need a simple but rough solution and then a more precise method later on. People have crashed planes when it is avoidable. The idea is to get the parameter to behave as the documentation says. Then the better more precise solution can be implemented. Key is to ensure this change isnt detrimental to current airframes.

I didn't even notice that it was reverted and in rework. Thank you for pointing that out.

I guess my opinion on this depends on timeline and functionality:

1. If WIP: AP_Airspeed: Blending of several sensors V2 #7891 is going to be an ongoing development for quite some time, I do think a quick fix is in order.
2. If WIP: AP_Airspeed: Blending of several sensors V2 #7891 addresses the same failure mode(s) as this pull request, then perhaps it can replace it when ready?
3. If this pull request addresses a failure mode which WIP: AP_Airspeed: Blending of several sensors V2 #7891 does not (I am making no assumptions), then it can either stand on its own or be an additional case addressed by WIP: AP_Airspeed: Blending of several sensors V2 #7891.

I think point number 2 is the most likely case from what I've read. Completely independent of these points however, is that AHRS_WIND_MAX does not currently function as intended. @EShamaev Do you think that the functionality as described implied in AHRS_WIND_MAX is undesirable in the long term? It doesn't prescribe a method for coping with the failing airspeed sensor, but just says that it does.

Then again, if the issue here is just how the comparison between ground speed and airspeed is made, that's yet another story. My take on that is my experience that a faulty airspeed sensor often results in wildly varying wind estimates for both magnitude and direction. So that may complicate a solution that takes direction into account.

EDIT: You know what, it sounds to me like only my last section is really applicable after reviewing all of the statements made by @EShamaev , but I'll leave the rest for completeness' sake.

@Evan1010 thank you for that, its basically how i view it too. Do you have some logs that may help, ive only got a handful and they seem to fail in the same way. Locked around a value with some minor noise. Ive had the issue but luckily i had arspd_use at 0 as i was testing.

I'll try to dig up some logs - they're from a while ago, after all.

@EShamaev I don't want to keep bothering you, but do you think the uncertainty in airspeed wind direction given by a failed sensor can be overcome for a more proper comparison between ground speed, air speed, and wind? As it is, I believe the code effectively assumes the worst case scenarios of a direct tailwind for the first comparison case (aspeed > (gnd_speed + wind_max)), and a direct headwind for the second comparison case (aspeed < (gnd_speed - wind_max)).

The wind estimate that I have seen during airspeed failures often does not climb quickly or even at all. When flying grids, it often oscillates near 0 because the estimated direction changes so often. Using the wind speed estimate is a bad idea right now.

I like that this PR matchs the documentation and parameter description, I think that AIRSPEED_USE should automatically be set to 0 in a single case - the absolute value of (immediate airspeed - ground speed) differ by more than AHRS_WIND_MAX, probably for about 3 seconds as proposed above. From my novice analysis of the code above, it looks like the calculation included is correct. This doesn't need to estimate wind vector or magnitude at all - It should just compare the airspeed and ground speed.

Later a "smarter" approach can be used using EKF at lower wind speeds, etc.

Hi @Naterater it is pulling it from the parameter. Where do you see it pulling it from ekf?

I added a helper function to ahrs lib to give me access to the parameters variable.

Sorry, I'm unfamiliar with helper functions but I'm happy that's what's going on. Will edit my post above again, please excuse the ignorance.

Hi @Naterater because its a private class variable only the class can access it. The function is public and it has access to the private variables.

I have added the auto recovery check after a period of time. Happy to change the figures over to '#define' instead so its better documentation wise. Will do that over the weekend.

I've done quite a bit of SITL testing. I've discovered there is a SIM_ARSPD_FAIL parameter too, as well as the other SIM_WIND_ methods that people have suggested. If we put a plane up and the operator gets the wind estimate wrong, it should seamlessly swtich between the two. A good way of knowing if you know your wind speeds I guess. The question is could this cause a plane to come down because the airspeed estimate is out?

So what I've done is I've made this use of the airspeed a type 3 use. I've had to rework a bunch of the existing functions to work but after doing some digging nothing else is using these functions.... Or they are used via other help functions which now account for the change.

This way if you want to have this mode of checking you can, otherwise it uses the normal throttle up and plough into the ground if your airspeed sensor fails. Having said that in testing I've had more altitude gains than ploughing into the ground......

One bug I'm tracking down, the recovery period seems to be quicker than the 9 seconds I've got it listed for, although I've not timed it specifically. Will be testing that some more.

I'll add a autotest case, update the param doco and test multiple airspeed sensors (I have no idea how this will behave) over the weekend.

Again theres some additional logic, please check that I've got this right - it looks like I do with the SITL testing.....

I'll come back to a longer term solution in a month or two unless there is a better one in the pipeline that will get across the line in the next 2 or 3 months? There a number of PR's that could meet that, so sing out if you intend to finish them off, or if not but you think the existing PR is a good starting point.

Still looking for logs for when this occurs, basically I've got only the failure type that @Naterater has and myself. Would really like to see more logs of this happening - I want to make sure we cover off all the failure scenarios encountered so far.

@magicrub I think I've applied most if not all your comments. Thanks for the suggestion of the low pass filter it has worked even better than I had thought.

I rejigged the code so that it does the filtering first, then does the handling of the filtered value. The filter enabled me to get rid of not only the counter variables but I rethought some of the logic which allowed me to further reduce them.

I've gone with an asymmetric variation on the filter so that the good readings impact the filtered value less. I've also added a warning level in so that if a person has a plane and they are out on their max wind speed max estimate by a small amount they may have time to fix it. I ran a quite a few SITL tests with various wind speeds, airspeed failures and turbulence levels. It handled exactly as I would expect.

I have far more confidence in the auto enable feature with the filtered value. It will take about 5 secs to trip a failure if they are all bad readings. It takes about 45 to 60 seconds to return to an ok state.

Hi @ChrisBird , thanks for working on this! It's getting much better. I re-wrote a few sections of it at pushed it to my repo branch "pitot_failsafe". Can you please look over this commit and test it?

magicrub@6596305

general changes:

• removed the dedicated file, it's just implementing one function. moved it ot the main cpp
• renamed a few things
• used a LPF properly for the probability (actually stays between 0 and 1)
• improved the warning trigger to only happen once, and ensure it always happens
• saved 200 bytes!
• exit sooner if there's no work to do
• sprinkled in some cost
• improved the bounds checking of .use

Hi @magicrub,

The additional file was requested on the previous dev call by Tridge as we'll be adding to the health checks in the near future. Its more about setting up the right framework for the future checks, so that its in position ready to go.

I'll have a look tonight and get back to you.

I re-added the file on another commit on my branch.

@magicrub, I've had a quick look. I still think some of the changes are not setting up the frame work for the future checks. The naming of enum items seem to support just one health check. I would envisage that there would eventually be different checks that people will be able to turn on, not just one health check. The KF version may not suite some people going forwards and they'd refer the simplistic check, or maybe they want both enabled. Not all will want the simple check once the KF version is done. I agree with most of the other changes, I'm going to test it now. What we have on by default will change as newer or better health checks come out. The airspeed health seems to have alot of different views and given that people appear to feel like this hasnt been addressed in years I think we should try to placate people a bit here. Of course once the more robust version is out then others will hopefully trust it more than the simple check.

per @ChrisBird 's request via gitter, he liked my version and was ok with me force-pushing it onto his branch.

We can remove the WIP tag. I think this PR is looking good. I'll do a squash in the morning and then if its good its ready to go.

I agree I'll but both in. In the future we'll add at least one extra check (based on synthetic airspeed)

I reluctantly agree to merge this as-is but let me state my case of why I prefer the other way.

The options check is better done inside check_sensor_failures() with it returning immediately if both are not enabled. This allows the same functionality but the AP_Airspeed::OptionsMask::ON_FAILURE_ defines would only ever be called inside check_sensor_ahrs_wind_max_failures() which makes it more obvious that it's use is confined there. Plus, if anyone else calls check_sensor_ahrs_wind_max_failures() in the future it ensures it's properly bypassed without doing unnecessary work and same behavior. For example, if someone else called this function in the future from somewhere else then probability would update where you version disables that update. Future coders are worthwhile to protect against :)

Also of note, my original rewrite did not check options until it used it because always updating the probability is useful. It probability should get logged too (new PR).

The whole thesis of this thread is that we should be allowing switch over to throttle mode in the event of airspeed failure. Should we be checking the sanity TRIM_THROTTLE during level cruise when the airspeed is healthy? That could be a potential future failsafe, but for now at least a warning would be quite easy to warn on the GCS "Caution: TRIM_THROTTLE exceeded in automatic level flight." It would be an indicator that AHRS_WIND_MAX is about to be exceeded if the wind isn't truly that high. Maybe this is just a feature to add to a future PR with more advanced failsafes, but I wanted to document the idea so I don't forget.

I've adding logging of the probability which is only useful when it's always being calculated so I went back to always running it, and only perform the actions if options says so. We can rework that later as we get more options if needed since we may be having different probabilities for different algorithms.

Meanwhile, it would be nice to see some real flight logs with the health probability with or without the feature enabled.

@Naterater yup, learning the trim param would come in handy if we're having to switch over to it. Good thing for another PR. meanwhile. MERGED!

Hi @magicrub, the reason why I think it shoudn't calculate it is say we have another checking method, say one that is working of a synthetic airspeed (like we have but use it to determine if its a solid reading or not). I'd like to use the same probability parameter, if I have the option off then I dont want it to modify the probability value, I only want those options that I have on to modify the probability.

So in the next go with the synthetic airspeed I'll split them out into two separate methods so that way they are decoupled, further. I'd see us having one method that looks at the probability and turns it on or off and then multiple method that may modify the probability. Does that make sense? Or should I approach this differently?

Yeah, best to separate the probability as a stand alone calc. At some point it may be useful to have multiple probabilities. If so, we will need to make a new logging msg.